Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jasmine updates #241

Merged
merged 10 commits into from
Feb 13, 2024
Merged

Jasmine updates #241

merged 10 commits into from
Feb 13, 2024

Conversation

clementzach
Copy link
Collaborator

Code to fix the jasmine out of range error

@biblicabeebli
Copy link
Member

As far as I can go with visualizing how these offsets work:
(longitude + 180) % 360 - 180 is completely fine because that is location on a circle, and should be handled as part of normal computation here.

For Latitude, we have the potential issue of "ticking up past 180" (uh, 360??) and suddenly the a correction clamping it by %360 swapping from north pole to south pole. (This issue is substantially unlikely to occur due to the restrictions on the magnitude of the offset - which I think is limited to 170 on both ios and android. (Literally I am bad at reading exactly that kind of behavior in code, it's just ... hard.) ---- I'm not seeing a corrective action on latitude.

I'm going to deploy this code to servers because I believe it resolves our current batch of errors.

I think this code needs to have tests for those cases where it could tick up and over. This is a safety thing, even if someone changes the code we need to know via a failing test that behavior has changed. We also need to somewhat more rigorously document the transformations occurring here in the code (and in the Android and iOS codebases) and declare why they are acceptable for Beiwe's purposes. For this reason I'm not going to merge it yet - we reference a commit over in the backend, not a branch name.

I'm slightly confused by one detail that came up in the email thread - it looks to me like our offset is no greater than one degree? that just seems wrong because it is ridiculously easy to de-anonamize - but literally this is not my field of expertise.

@clementzach do you want to set up a call so we can go through this verbally + with screen shares? It's very difficult to reason about without a secondary brain to sanity check, and particularly obnoxious to type out some of these details as text.

@clementzach
Copy link
Collaborator Author

My understanding is:

  1. Longitude (east-west) has valid values between -180 and 180. These can be changed any amount without any issue because moving North 1 degree at any point on the Equator is moving the same distance.

  2. Latitude (North/South) has valid values between -90 and 90. These cannot be changed very much because moving East 1 degree at the equator is a much bigger distance than moving East 1 degree at the North Pole. So, fuzzing for latitude is very small.

I think there may be more sophisticated methods of anonymizing data in the Beiwe app, but I think this method may be pretty close to good enough.

I agree with writing more unit tests and documentation. I'll send out an email to you and @jprince127 to set up a time.

@clementzach
Copy link
Collaborator Author

@jprince127 I added more documentation, I believe that this should be fine to merge unless you think there's something else that should be added

@jprince127 jprince127 merged commit 1c0e7bc into develop Feb 13, 2024
4 checks passed
@jprince127 jprince127 deleted the coerce_longitude_9_jan branch February 13, 2024 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants